Use subprocess timeout by ngie-eign · Pull Request #2127 · gitpython-developers/GitPython

ngie-eign · 2026-04-18T17:27:07Z

subprocess's APIs in 3.3+ support passing timeout to calls, such as
.communicate(..), .wait(..), etc. Pass kill_after_timeout to those
APIs and remove the watchdog handler code as it's not needed once
timeout= is used.

This enables kill_after_timeout on Windows platforms by side-effect as
upstream implements timeout for all supported platforms.

Requires: #2126

In the event the end-user called one of the APIs with `with_stdout=False`, i.e., they didn't want to capture stdout, the code would crash with an AttributeError or ValueError when trying to dereference the stdout/stderr streams attached to `Popen(..)` objects. Be more defensive by checking the streams first to make sure they're not `None` before trying to access their corresponding attributes. Add myself to AUTHORS and add corresponding regression tests for the change. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

Prior to this the test would fail [silently] on my macOS host during the test and then pytest would complain loudly about it being an issue post-session (regardless of whether or not the test was being run). Squash the unwritable directory to mute noise complaints from pytest. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

ngie-eign · 2026-04-18T17:55:31Z

The failure may have been caused by a sporadic network outage. Let's try again.

subprocess's APIs in 3.3+ support passing timeout to calls, such as .communicate(..), .wait(..), etc. Pass `kill_after_timeout` to those APIs and remove the watchdog handler code as it's not needed once `timeout=` is used. This enables `kill_after_timeout` on Windows platforms by side-effect as upstream implements `timeout` for all supported platforms. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

Copilot

Pull request overview

Updates process execution to rely on subprocess’s native timeout= support (instead of a custom watchdog) and adjusts related tests/cleanup.

Changes:

Remove watchdog-based timeout logic and use timeout= with Popen.communicate() / Popen.wait().
Add tests covering Git.execute(..., with_stdout=False) behavior (including with output_stream).
Improve test cleanup for unwritable directories/symlink scenarios; update AUTHORS.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.

File	Description
`git/cmd.py`	Replaces custom watchdog timeout with `subprocess` timeouts; adjusts stream handling and `AutoInterrupt` waits.
`test/test_git.py`	Adds regression tests for `with_stdout=False` with/without `output_stream`.
`test/test_util.py`	Adds a pytest finalizer to ensure cleanup of unwritable temp directories after the test.
`AUTHORS`	Adds contributor entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-19T04:27:28Z

                max_chunk_size = max_chunk_size if max_chunk_size and max_chunk_size > 0 else io.DEFAULT_BUFFER_SIZE
-                stream_copy(proc.stdout, output_stream, max_chunk_size)
-                stdout_value = proc.stdout.read()
-                stderr_value = proc.stderr.read()
+                if proc.stdout is not None:
+                    stream_copy(proc.stdout, output_stream, max_chunk_size)
+                    stdout_value = proc.stdout.read()


When output_stream is provided, the timeout is only applied to proc.wait(...) after stream_copy(...) finishes. If the child process hangs (or blocks writing) stream_copy can block indefinitely and kill_after_timeout will never be enforced. To preserve timeout semantics, consider using communicate(timeout=...) in this branch (writing captured stdout to output_stream), or implementing a non-blocking/selector-based copy loop that enforces the deadline while reading.

Copilot · 2026-04-19T04:27:28Z

+                    stdout_value = proc.stdout.read()
+                if proc.stderr is not None:
+                    stderr_value = proc.stderr.read()


In the output_stream branch, if with_stdout=False then proc.stdout is None and stdout_value remains the initial b"". In the non-output_stream branch, communicate() would return None for stdout in that case. Consider explicitly setting stdout_value = None when proc.stdout is None to keep return values consistent across branches.

Suggested change

stdout_value = proc.stdout.read()

if proc.stderr is not None:

stderr_value = proc.stderr.read()

stdout_value = proc.stdout.read()

else:

stdout_value = None

if proc.stderr is not None:

stderr_value = proc.stderr.read()

else:

stderr_value = None

Copilot · 2026-04-19T04:27:29Z

        try:
            if output_stream is None:
-                stdout_value, stderr_value = communicate()
+                stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout)


This change is intended to enable kill_after_timeout via the built-in timeout= support, including on Windows (per PR description), but Git.execute still rejects kill_after_timeout on Windows and the docstring still claims it is unsupported. Either the Windows guard/docs should be updated/removed, or the PR description should be adjusted to match the actual behavior.

Copilot · 2026-04-19T04:27:29Z

        try:
            proc.terminate()
-            status = proc.wait()  # Ensure the process goes away.
+            status = proc.wait(timeout=self.timeout)  # Ensure the process goes away.


_AutoInterrupt._terminate() now calls proc.wait(timeout=self.timeout). If a timeout is ever set, wait() can raise subprocess.TimeoutExpired, which is currently unhandled and could leave the process running (and self.proc already cleared). Consider catching TimeoutExpired and escalating to proc.kill() + a final wait() (possibly without a timeout) so termination is reliable.

Suggested change

status = proc.wait(timeout=self.timeout) # Ensure the process goes away.

try:

status = proc.wait(timeout=self.timeout) # Ensure the process goes away.

except subprocess.TimeoutExpired:

proc.kill()

status = proc.wait()

Copilot · 2026-04-19T04:27:29Z

+                    # Try lchmod first, if the platform supports it.
+                    symlink.lchmod(rwx)
+                except NotImplementedError:
+                    # The platform (probably win32) doesn't support lchmod; fall back to chmod.


pathlib.Path does not provide a lchmod() method on supported Python versions; this will raise AttributeError in the finalizer on platforms where this path is hit. If you need to chmod a symlink itself, use os.lchmod(...) where available, or Path.chmod(..., follow_symlinks=False) on Python versions that support it, with an AttributeError/NotImplementedError fallback.

Suggested change

# Try lchmod first, if the platform supports it.

symlink.lchmod(rwx)

except NotImplementedError:

# The platform (probably win32) doesn't support lchmod; fall back to chmod.

# Try to chmod the symlink itself, if the platform supports it.

if hasattr(os, "lchmod"):

os.lchmod(symlink, rwx)

else:

symlink.chmod(rwx, follow_symlinks=False)

except (AttributeError, NotImplementedError, TypeError):

# Fall back to chmod if symlink-specific chmod is unavailable.

Copilot · 2026-04-19T04:27:29Z


    :param kill_after_timeout:
-        :class:`float` or ``None``, Default = ``None``
+        :class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``.


The reST markup in the kill_after_timeout parameter doc is inconsistent (:class:int, float, or ...). Consider using literal markup consistently (e.g., int, float, None) to avoid broken generated docs.

Suggested change

:class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``.

``int``, ``float``, or ``None`` (block indefinitely), Default = ``None``.

Copilot · 2026-04-19T04:27:29Z

+                stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout)
                # Strip trailing "\n".


There are existing timeout tests for remote operations, but this change introduces new timeout behavior in Git.execute itself (e.g., communicate(timeout=...) / wait(timeout=...), handling of TimeoutExpired, behavior with output_stream). Consider adding/adjusting unit tests that exercise Git.execute(kill_after_timeout=...) directly, including the output_stream branch, to prevent regressions.

Copilot · 2026-04-19T04:27:30Z

+                stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout)
                # Strip trailing "\n".


proc.communicate(timeout=kill_after_timeout) can raise subprocess.TimeoutExpired, but that exception is not handled here. That means the process may be left running and callers will see an unexpected exception rather than the documented/previous behavior of killing the process and surfacing a GitCommandError with useful stderr. Consider catching TimeoutExpired, terminating/killing the process, collecting any remaining output, and then raising GitCommandError (or otherwise translating the timeout) consistently.

Byron · 2026-04-19T04:29:10Z

Thanks a lot for your continued work on this.

Let's take a look at the auto review before I merge it.

ngie-eign added 2 commits April 17, 2026 09:03

ngie-eign force-pushed the use-subprocess-timeout branch 2 times, most recently from 2d09ccf to 1435486 Compare April 18, 2026 17:29

ngie-eign marked this pull request as ready for review April 18, 2026 17:54

ngie-eign force-pushed the use-subprocess-timeout branch from 1435486 to 14190cb Compare April 18, 2026 17:55

ngie-eign marked this pull request as draft April 18, 2026 17:55

ngie-eign marked this pull request as ready for review April 18, 2026 18:05

ngie-eign mentioned this pull request Apr 18, 2026

git.cmd.Git.execute(..): fix with_stdout=False #2126

Merged

Byron requested a review from Copilot April 19, 2026 04:20

Copilot started reviewing on behalf of Byron April 19, 2026 04:20 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use subprocess timeout#2127

Use subprocess timeout#2127
ngie-eign wants to merge 3 commits intogitpython-developers:mainfrom
ngie-eign:use-subprocess-timeout

ngie-eign commented Apr 18, 2026

Uh oh!

ngie-eign commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

-                    # Try lchmod first, if the platform supports it.
-                    symlink.lchmod(rwx)
-                except NotImplementedError:
-                    # The platform (probably win32) doesn't support lchmod; fall back to chmod.
+                    # Try to chmod the symlink itself, if the platform supports it.
+                    if hasattr(os, "lchmod"):
+                        os.lchmod(symlink, rwx)
+                    else:
+                        symlink.chmod(rwx, follow_symlinks=False)
+                except (AttributeError, NotImplementedError, TypeError):
+                    # Fall back to chmod if symlink-specific chmod is unavailable.

	:class:`int`, `float`, or ``None`` (block indefinitely), Default = ``None``.
	``int``, ``float``, or ``None`` (block indefinitely), Default = ``None``.

		stdout_value, stderr_value = proc.communicate(timeout=kill_after_timeout)
		# Strip trailing "\n".

Uh oh!

Conversation

ngie-eign commented Apr 18, 2026

Uh oh!

ngie-eign commented Apr 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Byron commented Apr 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants